chore: deterministic backward compatibility tests#591
Open
kc1212 wants to merge 3 commits into
Open
Conversation
dvdplm
previously approved these changes
May 18, 2026
Contributor
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
A few nitpicks but otherwise it LGTM. I don't love the ALLOW_UNCOVERED stuff or the workspace scanning, but I also don't know of a better way to do this.
Comment on lines
+383
to
395
| ) -> Result<InternalCustodianSetupMessage, BackupError> { | ||
| let timestamp = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(); | ||
| self.generate_setup_message_with_timestamp(rng, custodian_name, timestamp) | ||
| } | ||
|
|
||
| // The timestamp is taken as an explicit argument so that callers needing deterministic | ||
| // output (e.g. backward-compatibility data generators) can pass a fixed value. | ||
| pub fn generate_setup_message_with_timestamp<R: Rng + CryptoRng>( | ||
| &self, | ||
| rng: &mut R, | ||
| custodian_name: String, | ||
| timestamp: u64, | ||
| ) -> Result<InternalCustodianSetupMessage, BackupError> { |
Contributor
There was a problem hiding this comment.
Do we really need both of these? Is generate_setup_message_with_timestamp really used separately from generate_setup_message? Maybe we can unify them into generate_setup_message and save ourselves some pub-API surface?
Contributor
Author
There was a problem hiding this comment.
it's a bit tricky, generate_setup_message_with_timestampbecause I wanted a "new" function that's deterministic, for the backward compatibility tests. but generate_setup_message is used everywhere else
| //! `with_fixed_int_encoding()` on top of `Versionize::versionize()`), and | ||
| //! asserts every output is byte-identical. | ||
| //! | ||
| //! The N-repeats pattern is what catches HashMap regressions: each freshly |
Contributor
There was a problem hiding this comment.
Hmm, what is it that we're actually testing here? It reads a bit like we're checking that HashMap is indeed non-deterministic and that BTreeMap is indeed deterministic? That'd be the concern of Rust's stdlib to do, i.e. that the documented invariants stay in place.
I don't think we need this. Am I wrong?
Contributor
Author
There was a problem hiding this comment.
it's more of a sanity check. on one hand it's not super useful for BTreeMap, but we also have functions like generate_setup_message_with_timestamp which is suppose to make de-randomize the initialization
Contributor
Author
There was a problem hiding this comment.
could be removed in the follow up - see the PR description
kc1212
force-pushed
the
kc1212/chore/3023/deterministic-backward-compatibility-tests
branch
from
734a02f to
6994051
Compare
Consolidated Tests Results 2026-05-21 - 13:17:24Test Results
Details
test-reporter: Run #4877
🎉 All tests passed!TestsView All Tests
🍂 No flaky tests in this run. Github Test Reporter by CTRF 💚 🔄 This comment has been updated |
kc1212
force-pushed
the
kc1212/chore/3023/deterministic-backward-compatibility-tests
branch
2 times, most recently
from
1c4e1fc to
2d33cc5
Compare
kc1212
force-pushed
the
kc1212/chore/3023/deterministic-backward-compatibility-tests
branch
from
2d33cc5 to
cea9f4e
Compare
Contributor
Author
Actually |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves determinism and workflow safety for backward-compatibility fixtures by (1) making new fixture generation byte-stable (notably by removing HashMap-driven nondeterminism and timestamp nondeterminism) and (2) preventing routine regeneration from touching historically frozen/non-deterministic versions.
Changes:
- Split backward-compatibility fixture versions into frozen vs deterministic, and restrict
generate-backward-compatibility-all/ cleaning to deterministic versions only. - Make
KeyGenMetadataInnerserialization deterministic by introducing a new version that usesBTreeMap(and updating call sites/tests accordingly). - Add a regression test suite that repeatedly builds/serializes representative fixtures to detect accidental nondeterminism.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Introduces frozen vs deterministic version lists; updates clean-* and generate-*-all to only affect deterministic versions. |
| docs/developer/backward_compatibility.md | Updates developer docs to explain frozen vs deterministic generation and new regeneration semantics. |
| core/service/tests/backward_compatibility_kms.rs | Updates test construction to use deterministic BTreeMap digests for current metadata paths. |
| core/service/tests/backward_compatibility_determinism.rs | Adds regression tests asserting byte-stable serialization across repeated fixture construction. |
| core/service/src/vault/storage/crypto_material/tests/migration.rs | Updates test metadata creation for new KeyGenMetadata::new signature (BTreeMap). |
| core/service/src/vault/storage/crypto_material/tests.rs | Updates helper metadata construction for new KeyGenMetadata::new signature (BTreeMap). |
| core/service/src/engine/utils.rs | Adjusts digest verification helper APIs to accept BTreeMap for deterministic iteration/serialization behavior. |
| core/service/src/engine/threshold/service/kms_impl.rs | Updates threshold engine tests to use new metadata constructor signature (BTreeMap). |
| core/service/src/engine/base.rs | Introduces KeyGenMetadataInner V2 with BTreeMap; adds upgrade path V1→V2 to preserve compatibility while ensuring determinism going forward. |
| core/service/src/backup/custodian.rs | Adds timestamp-injection API (generate_setup_message_with_timestamp) to support deterministic fixture creation. |
| backward-compatibility/generate-v0.14.0/src/generate.rs | Prevents duplicate .ron rows by replacing existing entries for regenerated versions instead of always appending. |
| backward-compatibility/ADDING_NEW_VERSIONS.md | Updates “adding versions” guide to reflect deterministic vs frozen approach and current generator baseline. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+36
to
+50
| # Frozen — generators may be non-deterministic. Their data is committed and | ||
| # must NOT be deleted or regenerated by `generate-backward-compatibility-all`. | ||
| FROZEN_BWC_VERSIONS := 0.11.0 0.11.1 0.13.0 0.13.10 0.13.20 | ||
|
|
||
| # Deterministic — re-running produces byte-identical output. | ||
| # `generate-backward-compatibility-all` cleans and regenerates these. | ||
| DETERMINISTIC_BWC_VERSIONS := 0.14.0 | ||
|
|
||
| # Derived data-dir paths (e.g. 0.14.0 -> backward-compatibility/data/0_14_0) | ||
| DETERMINISTIC_BWC_DIRS := $(addprefix backward-compatibility/data/,$(subst .,_,$(DETERMINISTIC_BWC_VERSIONS))) | ||
|
|
||
| clean-backward-compatibility-data: | ||
| rm -f backward-compatibility/data/kms.ron | ||
| rm -f backward-compatibility/data/kms-grpc.ron | ||
| rm -f backward-compatibility/data/threshold-fhe.ron | ||
| rm -rf backward-compatibility/data/0_11_0 | ||
| rm -rf backward-compatibility/data/0_11_1 | ||
| rm -rf backward-compatibility/data/0_13_0 | ||
| rm -rf backward-compatibility/data/0_13_10 | ||
| rm -rf backward-compatibility/data/0_13_20 | ||
| rm -rf backward-compatibility/data/0_14_0 | ||
|
|
||
| generate-backward-compatibility-v0.11.0: | ||
| cd backward-compatibility/generate-v0.11.0 && cargo run --release | ||
|
|
||
| generate-backward-compatibility-v0.11.1: | ||
| cd backward-compatibility/generate-v0.11.1 && cargo run --release | ||
| @# Only deterministic-version dirs are removed. Frozen dirs and the shared | ||
| @# .ron files are preserved so committed frozen entries survive. | ||
| @if [ -n "$(DETERMINISTIC_BWC_DIRS)" ]; then rm -rf $(DETERMINISTIC_BWC_DIRS); fi |
Comment on lines
46
to
+49
| make generate-backward-compatibility-all | ||
| ``` | ||
|
|
||
| Or generate for specific versions: | ||
| Or generate for a specific version with an existing Makefile target: |
| ``` | ||
|
|
||
| The `make generate-backward-compatibility-all` target cleans old data first, then runs all generators in sequence. | ||
| The `make generate-backward-compatibility-all` target only cleans and regenerates **deterministic** versions (listed in `DETERMINISTIC_BWC_VERSIONS` in the root `Makefile`). **Frozen** versions (`FROZEN_BWC_VERSIONS`, currently everything up to and including `0.13.20`) are left untouched — their data dirs are preserved and their generators are not invoked. The shared `.ron` files are never deleted by `-all`, so committed frozen entries survive across regenerations. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of changes
This is not exactly a complete fix because on main we don't have deterministic datatypes, it only introduces in this PR. There needs to be a follow up PR that changes the revision in
kms/backward-compatibility/generate-v0.14.0/Cargo.tomlafter this PR is merged.To test it locally, one can use this commit cea9f4e, but I didn't want to add it in this PR because it's not a commit on main.
Issue ticket number and link
Closes zama-ai/kms-internal#3023
PR Checklist
I attest that all checked items are satisfied. Any deviation is clearly justified above.
chore: ...).TODO(#issue).unwrap/expect/paniconly in tests or for invariant bugs (documented if present).devopslabel + infra notified + infra-team reviewer assigned.!and affected teams notified.Zeroize+ZeroizeOnDropimplemented.unsafe; if unavoidable: minimal, justified, documented, and test/fuzz covered.Dependency Update Questionnaire (only if deps changed or added)
Answer in the
Cargo.tomlnext to the dependency (or here if updating):More details and explanations for the checklist and dependency updates can be found in CONTRIBUTING.md